-
-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: 4575 - country is now always populated #4591
fix: 4575 - country is now always populated #4591
Conversation
final UserPreferences userPreferences, | ||
) async { | ||
const OpenFoodFactsCountry defaultCountry = | ||
OpenFoodFactsCountry.SWITZERLAND; // why not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one will be the subject to discussions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a country.
The normal behavior is to use the device's country.
If everything falls apart, only then will we use the default country. And the user sees in the onboarding that it can be changed to what is more relevant.
I have no problem changing the default country (I would even say "fallback country"). Just pick a "more relevant" country.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was exactly, the idea behind my comment.
What's the best value to use @teolemon and @stephanegigandet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case could it be null ? In that case, we should have a special screen appearing, asking to fix the issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be null only in once case: when the app is started the very first time, we need to pre-select the country of the user. For that we use some platform data (PlatformDispatcher.instance.locale.countryCode
). If this platform data is not available or is a code that does not match "our" countries, the country will be null.
Only in that case will we use the default country (so far, Switzerland).
And in the onboarding, the user will see the pre-selected country and will have the obvious possibility to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm proposing France, since we have contributors monitoring the influx of new products that could be a consequence of that
await userPreferences.setUserCountryCode(isoCode); | ||
} | ||
} | ||
|
||
// TODO(monsieurtanuki): move to off-dart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did already get answers in Ukrainian ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answers in Ukrainian, but regarding the UK for Eco-Score adjustments ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://fr.wikipedia.org/wiki/ISO_3166-1:
- Ukraine is
'ua'
- United Kingdom is
'gb'
For some reason, United Kingdom is 'uk'
in openfoodfacts.
final UserPreferences userPreferences, | ||
) async { | ||
const OpenFoodFactsCountry defaultCountry = | ||
OpenFoodFactsCountry.SWITZERLAND; // why not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenFoodFactsCountry.SWITZERLAND; // why not? | |
OpenFoodFactsCountry.FRANCE; // not ideal, but we have many contributors monitoring France |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teolemon OK for France. I haven't used your code suggestion because I wanted to avoid to reformat manually (your comment being too long).
I didn't work out, I have to reformat anyway...
We'll be ok in 5 minutes.
Codecov Report
@@ Coverage Diff @@
## develop #4591 +/- ##
===========================================
- Coverage 10.32% 10.26% -0.07%
===========================================
Files 296 297 +1
Lines 15410 15502 +92
===========================================
Hits 1591 1591
- Misses 13819 13911 +92
... and 27 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thank you @teolemon for your review! |
What
Fixes bug(s)